-
Notifications
You must be signed in to change notification settings - Fork 580
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sr: add basic auth support to the schema registry #6639
Conversation
if (rq.authn_method == config::rest_authn_method::http_basic) { | ||
// Will throw 400 & 401 if auth fails | ||
auto auth_result = rq.service().authenticator().authenticate( | ||
*rq.req); | ||
try { | ||
auth_result.require_authenticated(); | ||
} catch (const ss::httpd::base_exception& ex) { | ||
// The failure from require_authenticated() will throw 401 | ||
// instead of 403. So convert the exception to 403. | ||
if ( | ||
ex.status() == ss::httpd::reply::status_type::unauthorized) { | ||
throw ss::httpd::base_exception( | ||
ex.what(), ss::httpd::reply::status_type::forbidden); | ||
} else { | ||
// Something else went wrong | ||
throw; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your context server changes but I think I'll put them in #6626 after we get the initial feature PRs merged
f1244fb
to
a6fbe0d
Compare
Things left todo on this PR:
|
sounds like something we might potentilally be able to do in follow-up PR(s) but it's up to you. |
a6fbe0d
to
154be2b
Compare
154be2b
to
63737cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks awesome. just a couple super minor things
63737cc
to
c957334
Compare
BLOCKER: TimeQueryTest.test_timequery.batch_cache=True #6685 (comment) The failure is unrelated to this PR though since the failing test does not use:
|
/ci-repeat 5 |
Re-running CI now |
I restarted CI. debug run had the TimeQuery issue whose fix is landing soon, and release build ran out of disk space :( |
/ci-repeat 5 |
This commit tests HTTP Basic Authentication on the Schema Registry endpoint POST compatibility/subjects/{subject}/versions/{version}
This commit tests HTTP Basic Authentication on the Schema Registry endpoint DELETE /subjects/{subject}
This commit tests HTTP Basic Authentication on the Schema Registry endpoint DELETE /subjects/{subject}/versions/{version}
This commit tests HTTP Basic Authentication on the Schema Registry endpoint GET subjects/{subject}/versions/{version}/referencedBy
c957334
to
4154a81
Compare
CI is green now and the latest force push appears to contain changes that are completely disjoint with this PR |
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639 (cherry picked from commit d4bc3c8)
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639 (cherry picked from commit d4bc3c8)
With changes to authorization configuration options the CRD is updated. REF: redpanda-data#6639 (cherry picked from commit d4bc3c8)
Cover letter
At Redpanda there is an effort to improve security by supporting HTTP Basic Authentication on some of out services such as the Schema Registry. PR #6452 introduced HTTP Basic Authentication to the Pandaproxy but not the Schema Registry.
This PR adds Basic Auth support to all Schema Registry endpoints but it does not include support for multiple authenticated connections. That will come at a later time.
This PR includes configuration changes to enable basic auth:
The new configuration options to enable basic auth are attached to the listener in schema_registry_api:
Examples for the new authn config:
After this PR, the leftover tasks are:
/consumers
Backport Required
UX changes
Release notes
Features